Skip to content

Add more robust key processing: RSA#15015

Open
sjudson wants to merge 2 commits into
pyca:mainfrom
trail-of-forks:sj/robust-processing-rsa
Open

Add more robust key processing: RSA#15015
sjudson wants to merge 2 commits into
pyca:mainfrom
trail-of-forks:sj/robust-processing-rsa

Conversation

@sjudson

@sjudson sjudson commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

This PR adds more robust key processing for RSA by extending checks (e.g., that e != 1) to the PEM and DER load paths. To maintain a single set of checks across all paths, it updates the existing direct construction flow to also validate over the public key components as BigNums.

See #14992 for related changes to DSA processing.

Comment thread src/rust/src/backend/rsa.rs Outdated
n: &pyo3::Bound<'_, pyo3::types::PyInt>,
) -> CryptographyResult<()> {
if n.lt(3)? {
if n.lt(0)? {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check_public_key_components_from_py has error paths (one of which doesn't have the condition match the exception text), but then it calls check_public_key_components, which has the same error paths. This function also appears to only have one caller, so can we not just inline the py_int_to_bn calls and use the check_public_key_components path?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reaperhulk updated to remove the early exit, and consolidate all checks at the same layer.

CI is red, but as far as I can tell the error looks intermittent to me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a known bug in the RSA paths of Ubuntu 22.04 that they never backported the fix for, so yeah, just a flake. I'll review this soon, sorry for the delay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants